feat!: add componentDidCatch for error handling #60

bchelkowski · 2024-02-12T16:37:46Z

BREAKING CHANGE: Add enableKopytkoComponentDidCatch bs_const to the manifest file

What did you implement:

The new Lifecycle method componentDidCatch is invoked when the error is thrown within the component.

How did you implement it:

componentDidCatch(error as Object, info as Object)

  sub componentDidCatch(error as Object, info as Object)
    ' The Roku exception object
    ' https://developer.roku.com/docs/references/brightscript/language/error-handling.md#the-exception-object
    ?error
    ' The info object containing
    ' componentMethod - component method where the error has been thrown
    ' componentName - node name that extends KopytkoGroup or KopytkoLayoutGroup
    ?info
  end sub

How can we verify it:

Create a component, add componentDidCatch method, and make the component throw an error.

componentDidCatch implementation

sub componentDidCatch(error as Object, info as Object)
  ?info
  ?error
end sub

example of throwing an errorcomponentDidMount implementation

sub componentDidMount()
  variable = m.some.not.existing.nested.object
end sub

Todos:

Write documentation (if required)
Fix linting errors
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: YES

tomek-r · 2024-02-12T16:57:32Z

@bchelkowski I heard that try/catch has a huge performance penalty and should be used for instance for third-party libs. I did not benchmark myself. Have you?

What is the motivation to have such handler? When something breaks and app crashes its better to let the box crash rather to supress. At least in dev env.

bchelkowski · 2024-02-12T19:00:57Z

@tomek-r

I heard that try/catch has a huge performance penalty and should be used for instance for third-party libs. I did not benchmark myself. Have you?

I will try to benchmark it somehow. Could you link some resources that say so? I want to review them.

What is the motivation to have such handler? When something breaks and app crashes its better to let the box crash rather to supress. At least in dev env.

The motivation is to be able to address better app crashes.
This solution is not mandatory for Kopytko Components. Without writing the componentDidCatch method, the app will just behave as it was before (it will crash).
But if someone prefers to handle such crashes in their way, for example by showing a screen with an error message, or reporting the reason for the crash to their destination reporting platform.

So it does not answer the question should whether the app should crash or not, just gives the possibility to choose what should happen.

bchelkowski · 2024-02-13T07:36:40Z

I heard that try/catch has a huge performance penalty and should be used for instance for third-party libs. I did not benchmark myself. Have you?

I measured the most "fat" components in our application (using roTimespan). Of course, I don't have a big test group so it is not statistically the best approach, but if the performance of try catch would be bad or tragic I would see some differences, but I don't see any, with this solution, and without it looks the same.

tomek-r · 2024-02-13T08:18:14Z

@bchelkowski

Could you link some resources that say so? I want to review them.

I think it was some discussion with Roku support. Have you reviewed https://developer.roku.com/en-gb/docs/references/brightscript/language/error-handling.md#proper-usage-of-exceptions?

I measured the most "fat" components in our application (using roTimespan). Of course, I don't have a big test group so it is not statistically the best approach, but if the performance of try catch would be bad or tragic I would see some differences, but I don't see any, with this solution, and without it looks the same.

It looks like it is optimized for no crash scenario.

I am wondering if catching can be optional. In your proposal, if I don't override the componentDidCatch function try/catch is still executed. Can this be optional?
Does rethrowing change the source stack, for example shows the the file and line where you rethrow and not where the original crash happened?

bchelkowski · 2024-02-13T08:44:46Z

@tomek-r

I think it was some discussion with Roku support. Have you reviewed https://developer.roku.com/en-gb/docs/references/brightscript/language/error-handling.md#proper-usage-of-exceptions?

Yes, I based on this documentation.

I am wondering if catching can be optional. In your proposal, if I don't override the componentDidCatch function try/catch is still executed. Can this be optional?

Yes, we can introduce bs_const in the app manifest (however the bs_const is mandatory so it will be a breaking change).

Does rethrowing change the source stack, for example shows the the file and line where you rethrow and not where the original crash happened?

Good point, a crash is pointing to the rethrown line, which won't be a transparent solution.
So if we will introduce this feature it will be required to do it with bs_const.

tomek-r · 2024-02-13T09:33:57Z

@bchelkowski Can this be turned off by default?

I am wondering if we can have such handler per component meaning I can for instance have for a chosen component but not for all if bs_const feature flag is on.

bchelkowski · 2024-02-13T11:43:47Z

@tomek-r

What do you think about this one?
3987423

Without bs_const this implementation makes componentDidCatch transparent for current implementations.

tomek-r

@bchelkowski Wondering if we can inject #const to a file. But this would require each file modification via plugin?

tomek-r · 2024-02-13T11:52:48Z

src/components/renderer/Kopytko.brs

+sub _functionCall(func as Function, args = [] as Object, context = Invalid as Object)
+  if (context = Invalid) then context = GetGlobalAA()
+
+  argumentsNumber = args.count()
+  context["$$functionCall"] = func
+
+  if (argumentsNumber = 0)
+    context["$$functionCall"]()
+  else if (argumentsNumber = 1)
+    context["$$functionCall"](args[0])
+  else if (argumentsNumber = 2)
+    context["$$functionCall"](args[0], args[1])
+  end if
+
+  context.delete("$$functionCall")
+end sub


kopytko utils have function call function

True, will add utils function

tomek-r · 2024-02-13T11:53:28Z

src/components/renderer/Kopytko.brs

+    end try
+  else
+    _functionCall(func, args, context)


Less nesting if return is appended and no need for else.

No need for a return if there is else. 😃

I can change to return. It doesn't matter to me.

tomek-r · 2024-02-13T12:00:04Z

src/components/renderer/Kopytko.brs

@@ -2,6 +2,7 @@ sub init()
  m.state = {}
  m.elementToFocus = Invalid

+  m._enabledErrorHandling = Type(componentDidCatch) <> "<uninitialized>"


Suggested change

m._enabledErrorHandling = Type(componentDidCatch) <> "<uninitialized>"

m._enabledErrorCatching = Type(componentDidCatch) <> "<uninitialized>"

What do you think?

Without bs_const I won't be able to catch original error (during development for example). Working with supressed or rethrown errors might be a nightmare

What do you think?

I prefer Handling, but it doesn't matter to me so much, I can change to Catching.

Without bs_const I won't be able to catch original error (during development for example). Working with supressed or rethrown errors might be a nightmare

But if someone is suppressing the error for some reason, it shouldn't crash.

Some app components could always extend KopytkoGroup and have any logic the dev wants, it could be embedded there, so for example it could throw an error if they want.
And use this component instead of the KopytkoGroup.

For example, JavaScript suppressed error won't be thrown.

@tomek-r

For example, with the current solution, if you want to handle an error but have it thrown for the dev environment, you can create your component that will extend KopytkoGroup and use the MyAppGroup component instead of KopytkoGroup.

<?xml version="1.0" encoding="utf-8" ?> <component name="MyAppGroup" extends="KopytkoGroup"> <script type="text/brightscript" uri="MyAppGroup.brs" /> </component>

sub componentDidCatch(error as Object, info as Object) #if isDevelopmentEnvironment ?info throw error #end if ' ... normal production code end sub

Of course, it won't be the ideal solution as the backtrace of the crash won't be full,
but you still have the original error message and the original line that crashed.
The app is stopped also in the component scope (8085 port), so maybe you are not exactly at the line of the function that crashed, but you can check the component context and variables.

It is a trade-off solution I won't argue with that, but it is also an optional one, I don't want to force anybody to use it.

Maybe I will have some idea in the future how it could be improved but, don't have it now.

bchelkowski · 2024-02-13T12:26:10Z

@bchelkowski Wondering if we can inject #const to a file. But this would require each file modification via plugin?

I don't really follow. You mean some kind of new comment that will be converted to some kind of code value?

tomek-r · 2024-02-15T13:24:10Z

@bchelkowski

I don't really follow. You mean some kind of new comment that will be converted to some kind of code value?

So in Kopytko.brs file we can add a const with the value true/false and then depending on the value catch the function executions.

#const shouldUseTryCatch = false

sub initKopytko(dynamicProps = {} as Object, componentsMapping = {} as Object)
    if (m._isInitialized) then return

    m._kopytkoDOM.componentsMapping = componentsMapping

    m._previousProps = _cloneObject(dynamicProps)
    m.top.observeFieldScoped("focusedChild", "focusDidChange")
    m.top.update(dynamicProps)

    #if shouldUseTryCatch
        try 
            constructor()
         catch error
             componendDidCatch(error)
        end try
    #else
        constructor()
    #end if
    m._previousState = _cloneObject(m.state) ' required because of setting default state in constructor()

    _mountComponent()

    m._isInitialized = true
end sub

Something like that

# const shouldUseTryCatch = false

Would have to be appended during build time.

Also that means new flag would have to be needed for the config like useTryCatch: true/false

bchelkowski · 2024-02-16T08:27:35Z

Ok, but why actually? If someone doesn't define componentDidCatch for the component it won't use try/catch.

tomek-r · 2024-02-16T08:34:36Z

Image the requirement. On prod env I want to report errors to some external service and degradate grafecully so I define componentDidCatch. On dev env I want to have full, original stack in order to debug. Your proposition does not allow to do that. I don't want to comment out all componentDidCatch functions. I want to switch it off for all on dev env without modifying the source code.

bchelkowski · 2024-02-16T09:51:04Z

@tomek-r

Image the requirement. On prod env I want to report errors to some external service and degradate grafecully so I define componentDidCatch. On dev env I want to have full, original stack in order to debug. Your proposition does not allow to do that. I don't want to comment out all componentDidCatch functions. I want to switch it off for all on dev env without modifying the source code.

3b31ef8

tomek-r

Good stuff

# [3.0.0](v2.1.5...v3.0.0) (2024-02-16) * feat!: add componentDidCatch for error handling (#60) ([ea32b0e](ea32b0e)), closes [#60](#60) ### BREAKING CHANGES * Add enableKopytkoComponentDidCatch bs_const to the manifest file

github-actions · 2024-02-16T11:31:32Z

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: add componentDidCatch for error handling

36ee519

bchelkowski requested a review from a team as a code owner February 12, 2024 16:37

bchelkowski requested review from tbazelczuk and shirishapitta and removed request for a team February 12, 2024 16:37

feat: make componentDidCatch transparent for current implementations

3987423

fix: method name

2dd18d6

tomek-r reviewed Feb 13, 2024

View reviewed changes

chore: apply CR suggestions

05e2c6d

feat: add more info fields

8926608

bchelkowski force-pushed the feat--add-componentDidCatch branch from e7f311a to 8926608 Compare February 14, 2024 08:22

feat!: add enableKopytkoComponentDidCatch bs_const

3b31ef8

bchelkowski changed the title ~~feat: add componentDidCatch for error handling~~ feat!: add componentDidCatch for error handling Feb 16, 2024

chore: add _isComponentDidCatchEnabled

53f9bdb

bchelkowski force-pushed the feat--add-componentDidCatch branch from 135edbf to 53f9bdb Compare February 16, 2024 10:14

tomek-r approved these changes Feb 16, 2024

View reviewed changes

tbazelczuk approved these changes Feb 16, 2024

View reviewed changes

bchelkowski merged commit ea32b0e into master Feb 16, 2024
1 check passed

bchelkowski deleted the feat--add-componentDidCatch branch February 16, 2024 11:30

github-actions bot added the released label Feb 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add componentDidCatch for error handling #60

feat!: add componentDidCatch for error handling #60

bchelkowski commented Feb 12, 2024 •

edited

Loading

tomek-r commented Feb 12, 2024 •

edited

Loading

bchelkowski commented Feb 12, 2024 •

edited

Loading

bchelkowski commented Feb 13, 2024 •

edited

Loading

tomek-r commented Feb 13, 2024

bchelkowski commented Feb 13, 2024 •

edited

Loading

tomek-r commented Feb 13, 2024

bchelkowski commented Feb 13, 2024

tomek-r left a comment

tomek-r Feb 13, 2024

bchelkowski Feb 13, 2024

tomek-r Feb 13, 2024

bchelkowski Feb 13, 2024

tomek-r Feb 13, 2024

bchelkowski Feb 13, 2024 •

edited

Loading

bchelkowski Feb 14, 2024 •

edited

Loading

bchelkowski commented Feb 13, 2024

tomek-r commented Feb 15, 2024 •

edited

Loading

bchelkowski commented Feb 16, 2024

tomek-r commented Feb 16, 2024 •

edited

Loading

bchelkowski commented Feb 16, 2024 •

edited

Loading

tomek-r left a comment

github-actions bot commented Feb 16, 2024

	m._enabledErrorHandling = Type(componentDidCatch) <> "<uninitialized>"
	m._enabledErrorCatching = Type(componentDidCatch) <> "<uninitialized>"

feat!: add componentDidCatch for error handling #60

feat!: add componentDidCatch for error handling #60

Conversation

bchelkowski commented Feb 12, 2024 • edited Loading

What did you implement:

How did you implement it:

How can we verify it:

Todos:

tomek-r commented Feb 12, 2024 • edited Loading

bchelkowski commented Feb 12, 2024 • edited Loading

bchelkowski commented Feb 13, 2024 • edited Loading

tomek-r commented Feb 13, 2024

bchelkowski commented Feb 13, 2024 • edited Loading

tomek-r commented Feb 13, 2024

bchelkowski commented Feb 13, 2024

tomek-r left a comment

Choose a reason for hiding this comment

tomek-r Feb 13, 2024

Choose a reason for hiding this comment

bchelkowski Feb 13, 2024

Choose a reason for hiding this comment

tomek-r Feb 13, 2024

Choose a reason for hiding this comment

bchelkowski Feb 13, 2024

Choose a reason for hiding this comment

tomek-r Feb 13, 2024

Choose a reason for hiding this comment

bchelkowski Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

bchelkowski Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

bchelkowski commented Feb 13, 2024

tomek-r commented Feb 15, 2024 • edited Loading

bchelkowski commented Feb 16, 2024

tomek-r commented Feb 16, 2024 • edited Loading

bchelkowski commented Feb 16, 2024 • edited Loading

tomek-r left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2024

bchelkowski commented Feb 12, 2024 •

edited

Loading

tomek-r commented Feb 12, 2024 •

edited

Loading

bchelkowski commented Feb 12, 2024 •

edited

Loading

bchelkowski commented Feb 13, 2024 •

edited

Loading

bchelkowski commented Feb 13, 2024 •

edited

Loading

bchelkowski Feb 13, 2024 •

edited

Loading

bchelkowski Feb 14, 2024 •

edited

Loading

tomek-r commented Feb 15, 2024 •

edited

Loading

tomek-r commented Feb 16, 2024 •

edited

Loading

bchelkowski commented Feb 16, 2024 •

edited

Loading